On Mon, Jul 25, 2022 at 7:39 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > This patch makes the backup history filename check more tight. > > Can you please elaborate a bit on the issue with existing > IsBackupHistoryFileName(), if there's any? > There are two call of this function, `CleanupBackupHistory` and `SetWALFileNameForCleanup`, there seems no issue of the existing IsBackupHistoryFileName() since the creation of the backup history file will make sure it is of format "%08X%08X%08X.%08X.backup". The patch just makes `IsBackupHistoryFileName()` more match to `BackupHistoryFileName()`, thus more easier to understand. > Also, the patch does have hard coded numbers [1] which isn't good from > a readability perspective, adding macros and/or comments would help > here. I'm not sure using macros is a good idea here, cause I noticed `IsTLHistoryFileName()` also uses some hard code numbers [1]. [1] static inline bool IsTLHistoryFileName(const char *fname) { return (strlen(fname) == 8 + strlen(".history") && strspn(fname, "0123456789ABCDEF") == 8 && strcmp(fname + 8, ".history") == 0); } > > [1] > static inline bool > IsBackupHistoryFileName(const char *fname) > { > - return (strlen(fname) > XLOG_FNAME_LEN && > + return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") && > strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && > - strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0); > + strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 && > + strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0); > } > > Regards, > Bharath Rupireddy. -- Regards Junwang Zhao